Add functional test for java-spiffe-helper #207

moritzschmitz-oviva · 2024-02-14T07:05:28Z

Closes #200

Build the java-spiffe-helper container using the latest changes on the branch.
Bootstrap a kind cluster and load the previously built image into it.
Install the spire-server chart with dependencies. Set special values for the CN in the certificates. This is what we later test.
Start the java-spiffe-helper container and let it do its magic.
Copy the created *.jks files and verify the CN values set earlier.

Potential discussion topics:

Using the latest spire charts. Perhaps pin the version here?
Checking only for the CN. Anything else to check for? For example the properties are using hard-coded values. Could also make them dynamic.

kfox1111

This looks good to me. :)

One possible change inline.

.github/workflows/test.yaml

kfox1111 · 2024-02-14T17:44:29Z

Looks good to me. :)

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

maxlambrecht · 2024-02-28T22:18:26Z

.github/workflows/test.yaml

@@ -0,0 +1,59 @@
+name: Test


Suggested change

name: Test

name: Java SPIFFE Helper CI

maxlambrecht · 2024-02-28T22:39:13Z

.github/workflows/test.yaml

+      - if: ${{ failure() }}
+        run: kubectl logs pod/java-spiffe-helper | tee java-spiffe-helper.log
+      - if: ${{ failure() }}
+        uses: actions/upload-artifact@v4
+        with:
+          name: logs
+          path: java-spiffe-helper.log
+      - if: ${{ failure() }}
+        run: kubectl describe pod/java-spiffe-helper
+      - uses: actions/setup-java@v4
+        with:
+          distribution: 'temurin'
+          java-version: '17'
+      - run: kubectl cp java-spiffe-helper:/tmp/keystore.p12 keystore.p12
+      - run: kubectl cp java-spiffe-helper:/tmp/truststore.p12 truststore.p12
+      - run: keytool -v -list -keystore keystore.p12 -storepass password | grep "CN=${{ env.KEYSTORE_COMMON_NAME }}"
+      - if: ${{ failure() }}
+        run: keytool -v -list -keystore keystore.p12 -storepass password
+      - run: keytool -v -list -keystore truststore.p12 -storepass password | grep "CN=${{ env.TRUSTSTORE_COMMON_NAME }}"
+      - if: ${{ failure() }}
+        run: keytool -v -list -keystore truststore.p12 -storepass password


I'd suggest adding step names to improve clarity, something like:

Suggested change

- if: ${{ failure() }}

run: kubectl logs pod/java-spiffe-helper | tee java-spiffe-helper.log

- if: ${{ failure() }}

uses: actions/upload-artifact@v4

with:

name: logs

path: java-spiffe-helper.log

- if: ${{ failure() }}

run: kubectl describe pod/java-spiffe-helper

- uses: actions/setup-java@v4

with:

distribution: 'temurin'

java-version: '17'

- run: kubectl cp java-spiffe-helper:/tmp/keystore.p12 keystore.p12

- run: kubectl cp java-spiffe-helper:/tmp/truststore.p12 truststore.p12

- run: keytool -v -list -keystore keystore.p12 -storepass password | grep "CN=${{ env.KEYSTORE_COMMON_NAME }}"

- if: ${{ failure() }}

run: keytool -v -list -keystore keystore.p12 -storepass password

- run: keytool -v -list -keystore truststore.p12 -storepass password | grep "CN=${{ env.TRUSTSTORE_COMMON_NAME }}"

- if: ${{ failure() }}

run: keytool -v -list -keystore truststore.p12 -storepass password

- name: Capture Pod Logs on Failure

if: ${{ failure() }}

run: kubectl logs pod/java-spiffe-helper | tess java-spiffe-helper.log

- name: Upload Pod Logs on Failure

if: ${{ failure() }}

uses: actions/upload-artifact@v4

with:

name: java-spiffe-helper-logs

path: java-spiffe-helper.log

- name: Describe Pod on Failure

if: ${{ failure() }}

run: kubectl describe pod/java-spiffe-helper

- name: Setup Java Environment

uses: actions/setup-java@v4

with:

distribution: 'temurin'

java-version: '17'

- name: Copy Keystore and Truststore from Pod

run: |

kubectl cp java-spiffe-helper:/tmp/keystore.p12 keystore.p12

kubectl cp java-spiffe-helper:/tmp/truststore.p12 truststore.p12

- name: Verify Keystore Common Name

run: keytool -v -list -keystore keystore.p12 -storepass ${{ secrets.KEYSTORE_PASSWORD }} | grep "CN=${{ env.KEYSTORE_COMMON_NAME }}"

if: ${{ failure() }}

run: keytool -v -list -keystore keystore.p12 -storepass ${{ secrets.KEYSTORE_PASSWORD }}

- name: Verify Truststore Common Name

run: keytool -v -list -keystore truststore.p12 -storepass ${{ secrets.TRUSTSTORE_PASSWORD }} | grep "CN=${{ env.TRUSTSTORE_COMMON_NAME }}"

if: ${{ failure() }}

run: keytool -v -list -keystore truststore.p12 -storepass ${{ secrets.TRUSTSTORE_PASSWORD }}

maxlambrecht · 2024-02-28T22:43:43Z

.github/workflows/test.yaml

+      - uses: helm/kind-action@v1
+        with:
+          cluster_name: kind
+      - run: kind load docker-image java-spiffe-helper:test --name kind
+      - run: helm upgrade --install -n spire-server spire-crds spire-crds --repo https://spiffe.github.io/helm-charts-hardened/ --create-namespace
+      - run: |
+          helm upgrade --install -n spire-server spire spire \
+            --repo https://spiffe.github.io/helm-charts-hardened/ \
+            -f .github/tests/spire-values.yaml \
+            --set spire-server.ca_subject.common_name="$TRUSTSTORE_COMMON_NAME" \
+            --set spire-server.controllerManager.identities.clusterSPIFFEIDs.java-spiffe-helper.dnsNameTemplates[0]="$KEYSTORE_COMMON_NAME"
+      - run: kubectl apply -f .github/tests/java-spiffe-helper.yaml
+      - run: kubectl wait pod/java-spiffe-helper --for condition=Ready --timeout=90s


This section could benefit from added clarity through the use of descriptive step names and breaking down complex commands for better readability:

Suggestion:

Suggested change

- uses: helm/kind-action@v1

with:

cluster_name: kind

- run: kind load docker-image java-spiffe-helper:test --name kind

- run: helm upgrade --install -n spire-server spire-crds spire-crds --repo https://spiffe.github.io/helm-charts-hardened/ --create-namespace

- run: |

helm upgrade --install -n spire-server spire spire \

--repo https://spiffe.github.io/helm-charts-hardened/ \

-f .github/tests/spire-values.yaml \

--set spire-server.ca_subject.common_name="$TRUSTSTORE_COMMON_NAME" \

--set spire-server.controllerManager.identities.clusterSPIFFEIDs.java-spiffe-helper.dnsNameTemplates[0]="$KEYSTORE_COMMON_NAME"

- run: kubectl apply -f .github/tests/java-spiffe-helper.yaml

- run: kubectl wait pod/java-spiffe-helper --for condition=Ready --timeout=90s

- name: Setup Kubernetes in Docker (KIND)

uses: helm/kind-action@v1

with:

cluster_name: kind

- name: Load Docker Image into KIND

run: kind load docker-image java-spiffe-helper:test --name kind

- name: Install SPIRE CRDs via Helm

run: helm upgrade --install -n spire-server spire-crds spire-crds --repo https://spiffe.github.io/helm-charts-hardened/ --create-namespace

- name: Configure and Install SPIRE Server

run: |

helm upgrade --install -n spire-server spire spire \

--repo https://spiffe.github.io/helm-charts-hardened/ \

-f .github/tests/spire-values.yaml \

--set spire-server.ca_subject.common_name="$TRUSTSTORE_COMMON_NAME" \

--set spire-server.controllerManager.identities.clusterSPIFFEIDs.java-spiffe-helper.dnsNameTemplates[0]="$KEYSTORE_COMMON_NAME"

- name: Apply java-spiffe-helper Kubernetes Configuration

run: kubectl apply -f .github/tests/java-spiffe-helper.yaml

- name: Wait for java-spiffe-helper Pod Readiness

run: kubectl wait pod/java-spiffe-helper --for condition=Ready --timeout=90s

maxlambrecht · 2024-02-28T22:44:03Z

Hey @moritzschmitz-oviva, thanks a lot for this contribution!

I've reviewed the changes, left a few comments and have a couple of additional suggestions:

Could you please rename the test.yaml file to java-spiffe-helper-ci.yaml? This name better reflects its purpose within our CI pipeline.
Also, it would be great if we could rename the folder from github/workflows/tests to github/workflows/ci-k8s-configs.

In addition, I believe it's important to cover all supported LTS versions of Java to ensure compatibility. Could we update the job configuration to test against the following Java versions?

    name: Test on Java ${{ matrix.java_version }}
    runs-on: ubuntu-latest
    strategy:
      matrix:
        java_version: [8, 11, 17, 21]  # Including all current LTS versions

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

moritzschmitz-oviva · 2024-02-29T20:53:19Z

Could you please rename the test.yaml file to java-spiffe-helper-ci.yaml? This name better reflects its purpose within our CI pipeline.

Done

Also, it would be great if we could rename the folder from github/workflows/tests to github/workflows/ci-k8s-configs.

Done

In addition, I believe it's important to cover all supported LTS versions of Java to ensure compatibility. Could we update the job configuration to test against the following Java versions?

I don't think this is necessary. The actions/setup-java in the workflow is only required for keytool. java-spiffe-helper itself is containerized and built with the same Dockerfile which is used for the 'main' build as well.

maxlambrecht · 2024-02-29T22:50:25Z

I don't think this is necessary. The actions/setup-java in the workflow is only required for keytool. java-spiffe-helper itself is containerized and built with the same Dockerfile which is used for the 'main' build as well.

You are completely right!

maxlambrecht · 2024-02-29T22:50:55Z

Hey @moritzschmitz-oviva, this PR looks good! I agree with the point you raised in the 'topics to discuss' about pinning the chart version. It's definitely a good practice that can enhance the stability of the test. Looking forward to seeing this adjustment.

As for the CN verification, it appears to be sufficient for the scope of this functional test. I'm particularly interested in seeing the java-spiffe-helper in action storing certificates in a JKS file, and then ensuring the expected cert is stored.

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

moritzschmitz-oviva · 2024-03-01T07:42:21Z

Hey @moritzschmitz-oviva, this PR looks good! I agree with the point you raised in the 'topics to discuss' about pinning the chart version. It's definitely a good practice that can enhance the stability of the test. Looking forward to seeing this adjustment.

Adjusted the workflow. Perhaps over-engineered it a bit now. Adding additional chart versions should be straight forward for testing. I didn't pin the patch, because according to SemVer, patches must be backwards compatible changes.

maxlambrecht

LGTM!

Thanks, @moritzschmitz-oviva !

moritzschmitz-oviva force-pushed the add-functional-test-for-java-spiffe-helper branch 2 times, most recently from ac58a5b to d2c5d32 Compare February 14, 2024 10:59

moritzschmitz-oviva marked this pull request as ready for review February 14, 2024 11:00

kfox1111 reviewed Feb 14, 2024

View reviewed changes

.github/workflows/test.yaml Outdated Show resolved Hide resolved

maxlambrecht requested review from maxlambrecht and rturner3 February 14, 2024 16:02

moritzschmitz-oviva requested a review from kfox1111 February 14, 2024 17:42

moritzschmitz-oviva force-pushed the add-functional-test-for-java-spiffe-helper branch from dfc4a16 to 0158e34 Compare February 16, 2024 13:06

moritzschmitz-oviva added 5 commits February 19, 2024 15:31

Add functional test for java-spiffe-helper

8d4b46f

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

Only run on pull_request

a0c6669

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

No need for selector, there is only one pod

bb86033

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

Implement suggested changes

56fa0de

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

Use tee for logs

b71d80a

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

moritzschmitz-oviva force-pushed the add-functional-test-for-java-spiffe-helper branch from 0158e34 to b71d80a Compare February 19, 2024 14:31

Merge branch 'main' into add-functional-test-for-java-spiffe-helper

a7e2ca9

maxlambrecht reviewed Feb 28, 2024

View reviewed changes

A few changes according to comments

c60431e

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

moritzschmitz-oviva requested a review from maxlambrecht February 29, 2024 20:53

moritzschmitz-oviva added 3 commits March 1, 2024 07:55

Extend workflow to support multiple semver ranges for SPIRE helm charts

70213da

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

Extend workflow to support multiple semver ranges for SPIRE helm charts

7e99bdc

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

USe SemVer range instead of fixed version

9ec94cd

Signed-off-by: Moritz Schmitz von Hülst <[email protected]>

maxlambrecht approved these changes Mar 1, 2024

View reviewed changes

maxlambrecht merged commit 616e8bc into spiffe:main Mar 1, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functional test for java-spiffe-helper #207

Add functional test for java-spiffe-helper #207

moritzschmitz-oviva commented Feb 14, 2024 •

edited

Loading

kfox1111 left a comment

kfox1111 commented Feb 14, 2024

maxlambrecht Feb 28, 2024

maxlambrecht Feb 28, 2024

maxlambrecht Feb 28, 2024

maxlambrecht commented Feb 28, 2024 •

edited

Loading

moritzschmitz-oviva commented Feb 29, 2024

maxlambrecht commented Feb 29, 2024

maxlambrecht commented Feb 29, 2024 •

edited

Loading

moritzschmitz-oviva commented Mar 1, 2024

maxlambrecht left a comment

Add functional test for java-spiffe-helper #207

Add functional test for java-spiffe-helper #207

Conversation

moritzschmitz-oviva commented Feb 14, 2024 • edited Loading

kfox1111 left a comment

Choose a reason for hiding this comment

kfox1111 commented Feb 14, 2024

maxlambrecht Feb 28, 2024

Choose a reason for hiding this comment

maxlambrecht Feb 28, 2024

Choose a reason for hiding this comment

maxlambrecht Feb 28, 2024

Choose a reason for hiding this comment

maxlambrecht commented Feb 28, 2024 • edited Loading

moritzschmitz-oviva commented Feb 29, 2024

maxlambrecht commented Feb 29, 2024

maxlambrecht commented Feb 29, 2024 • edited Loading

moritzschmitz-oviva commented Mar 1, 2024

maxlambrecht left a comment

Choose a reason for hiding this comment

moritzschmitz-oviva commented Feb 14, 2024 •

edited

Loading

maxlambrecht commented Feb 28, 2024 •

edited

Loading

maxlambrecht commented Feb 29, 2024 •

edited

Loading